-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(exex): WAL handle #11266
feat(exex): WAL handle #11266
Conversation
crates/exex/exex/src/wal/cache.rs
Outdated
pub struct BlockCache { | ||
/// A mapping of `File ID -> List of Blocks`. | ||
/// | ||
/// For each notification written to the WAL, there will be an entry per block written to | ||
/// the cache with the same file ID. I.e. for each notification, there may be multiple blocks | ||
/// in the cache. | ||
files: BTreeMap<u64, VecDeque<CachedBlock>>, | ||
files: Arc<RwLock<BTreeMap<u64, VecDeque<CachedBlock>>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will likely be removed, i don't think that we need an ordered mapping anymore, according to #11202
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some questions
crates/exex/exex/src/wal/cache.rs
Outdated
#[derive(Debug, Clone)] | ||
pub struct BlockCache { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this type now has two locked fields.
arent those two fields in sync, meaning files
and blocks
have the same entries?
not obvious from reading the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously we've deleted the notifications from WAL by walking them in an order they were inserted, so we needed a BTreeMap
indexed by the file ID
we won't need it anymore and the files
will be removed https://github.com/paradigmxyz/reth/pull/11266/files#r1777654333
basically, having a mapping of blocks (either hashes to file IDs, or numbers to file IDs) is enough for our new design #11202 because we don't rely on the order of files anymore and walk by hashes
@@ -310,6 +311,8 @@ pub async fn test_exex_context_with_chain_spec( | |||
components.provider.clone(), | |||
components.components.executor.clone(), | |||
notifications_rx, | |||
// TODO(alexey): do we want to expose WAL to the user? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think so
@@ -96,14 +106,6 @@ impl<Node: FullNodeComponents + Clone> ExExLauncher<Node> { | |||
// spawn exex manager | |||
debug!(target: "reth::cli", "spawning exex manager"); | |||
// todo(onbjerg): rm magic number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated, outdated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated, #11277
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pending parking lot semaphores
Towards #11261
WalHandle
doesn't have any methods yet, this PR just makes it clonable and passes it to the notifications structs